chore: disallow overriding of internal tool implementations #766
Conversation
himanshusinghs
added
the
no-title-validation
There was a problem hiding this comment.
Pull request overview
This PR refactors the tool registration system to prevent external tools from overriding internal tool implementations. The main change is renaming the tools parameter to additionalTools throughout the codebase, making it clear that custom tools are registered in addition to, rather than instead of, the default tools. Additionally, it implements a name collision detection mechanism to prevent registering tools with duplicate names.
Key Changes:
- Renamed
toolsparameter toadditionalToolsacross the server, transport, and test files - Added collision detection in
registerTools()to throw errors when duplicate tool names are detected - Updated test expectations to reflect that custom tools now supplement rather than replace default tools
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/server.ts |
Implements core changes: renames tools to additionalTools, separates internal/additional tool implementations, and adds name collision detection logic |
src/transports/base.ts |
Updates transport runner to use additionalTools parameter name and passes it through to server construction |
tests/integration/customTools.test.ts |
Updates test to verify custom tools are registered alongside (not replacing) default tools |
tests/integration/tools/mongodb/mongodbTool.test.ts |
Updates test setup to use additionalTools parameter and adds new test for name collision detection |
tests/integration/tools/mongodb/create/createIndex.test.ts |
Adjusts assertion to accept both "PENDING" and "BUILDING" statuses for vector search index |
|
Lacking context about the conversation, what is the concern here? If they want to override the default tools, I don't see why they shouldn't be able to. |
|
For context - we discussed that we already have means to replace the internal tools (using disabledTools and adding new tools) and having another method does not make sense. The usecase of overriding our internal tools is still supported, users just need to disable the internal tool and provide their own implementation but under a different name of their choosing. I don't think disabling tools is a hack, its a feature we have supported all the way so far. |
himanshusinghs
force-pushed
the
chore/disallow-overriding-of-internal-tools
branch
2 times, most recently
from
54a2eab to
b232a56
Compare
Pull Request Test Coverage Report for Build 19743479350Details
💛 - Coveralls |
|
|
||
| export class ConnectClusterTool extends AtlasToolBase { | ||
| public name = "atlas-connect-cluster"; | ||
| static toolName = "atlas-connect-cluster"; |
There was a problem hiding this comment.
ConnectClusterTool.toolName? it's better to avoid stutter
There was a problem hiding this comment.
The static property name is unfortunately reserved for Function.name and TS forbids overriding that.
To allow library consumers to make use of existing tool names, category and operationType, we are marking the name, category and operationType properties of a Tool as static so that library consumers can simply import AllTools from mongodb-mcp-server/tools export and make decision based on these static properties.
himanshusinghs
force-pushed
the
chore/disallow-overriding-of-internal-tools
branch
from
b232a56 to
9cc5526
Compare
|
Following an internal conversation, we will continue to support the existing interface of allowing library users to specify tool implementations while ensuring that tools have unique names during tool registration. This PR will be closed and the change for implementing uniqueness will be added to #767 |
Proposed changes
This PR follows up on recent conversation regarding tool implementations and the idea that we should not allow library consumers to remove internal tool implementations and if they wish to disable internal tools, they can do so using the session config hook.
Following the outcome of the same conversation, this implements a name collision check to ensure we don't register same named tools more than once.
Additionally, the last commit (9cc5526), moves the
name,operationTypeandcategoryto static properties of ToolClass so that library consumers have a chance of disabling multiple tools conditionally without statically typing the tool names.I would advice reviewing the PR in two different steps:
name,operationTypeandcategoryto static properties of ToolClass to make it easy for library consumers, the last commit - Link hereChecklist